feat: persist provider connection status#26
Conversation
Store provider connection test results in a separate backend status file and reload them from the desktop UI. Reset stale status entries when provider connection fields change. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces connection testing capabilities for ASR and LLM providers, adding UI elements to the settings page, localization strings, and backend Tauri commands to execute and persist test results. The review feedback highlights a critical compilation error on stable Rust caused by the use of unstable let_chains syntax. Additionally, the reviewer recommends refactoring the status management to utilize an in-memory store within AppState to prevent race conditions and disk I/O overhead, and suggests awaiting configuration saves prior to testing connections to ensure draft settings are correctly applied.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // 2. Reset stale provider test status when connection settings changed | ||
| let mut provider_status = load_provider_status(&state.provider_status_path); | ||
| if reset_changed_provider_status(&old_config, &config, &mut provider_status) | ||
| && let Err(e) = save_provider_status(&state.provider_status_path, &provider_status) | ||
| { | ||
| tracing::warn!( | ||
| "failed to save provider status to {}: {}", | ||
| state.provider_status_path.display(), | ||
| e | ||
| ); | ||
| } |
There was a problem hiding this comment.
Avoid using the unstable let_chains syntax (&& let Err(e) = ...) which will cause a compilation error on stable Rust. Instead, use a standard nested if let block. Additionally, use the in-memory provider_status store from AppState to prevent race conditions.
| // 2. Reset stale provider test status when connection settings changed | |
| let mut provider_status = load_provider_status(&state.provider_status_path); | |
| if reset_changed_provider_status(&old_config, &config, &mut provider_status) | |
| && let Err(e) = save_provider_status(&state.provider_status_path, &provider_status) | |
| { | |
| tracing::warn!( | |
| "failed to save provider status to {}: {}", | |
| state.provider_status_path.display(), | |
| e | |
| ); | |
| } | |
| // 2. Reset stale provider test status when connection settings changed | |
| let mut provider_status = state.provider_status.lock().unwrap(); | |
| if reset_changed_provider_status(&old_config, &config, &mut provider_status) { | |
| if let Err(e) = save_provider_status(&state.provider_status_path, &provider_status) { | |
| tracing::warn!( | |
| "failed to save provider status to {}: {}", | |
| state.provider_status_path.display(), | |
| e | |
| ); | |
| } | |
| } |
| struct AppState { | ||
| config: Mutex<AppConfig>, | ||
| config_path: std::path::PathBuf, | ||
| provider_status_path: std::path::PathBuf, |
There was a problem hiding this comment.
Add provider_status to AppState to keep the connection test statuses in memory. This allows safe, synchronized access and avoids concurrent file read/write race conditions.
| provider_status_path: std::path::PathBuf, | |
| provider_status_path: std::path::PathBuf, | |
| provider_status: Mutex<ProviderStatusStore>, |
| fn get_provider_connection_status( | ||
| state: tauri::State<AppState>, | ||
| kind: String, | ||
| connection_id: String, | ||
| ) -> Option<ProviderConnectionStatus> { | ||
| let store = load_provider_status(&state.provider_status_path); | ||
| provider_status_for_kind(&store, kind.trim()) | ||
| .and_then(|statuses| statuses.get(connection_id.trim()).cloned()) | ||
| } | ||
|
|
There was a problem hiding this comment.
Read the connection test status directly from the in-memory provider_status store in AppState instead of loading it from disk on every request. This is much faster and safer.
| fn get_provider_connection_status( | |
| state: tauri::State<AppState>, | |
| kind: String, | |
| connection_id: String, | |
| ) -> Option<ProviderConnectionStatus> { | |
| let store = load_provider_status(&state.provider_status_path); | |
| provider_status_for_kind(&store, kind.trim()) | |
| .and_then(|statuses| statuses.get(connection_id.trim()).cloned()) | |
| } | |
| #[tauri::command] | |
| fn get_provider_connection_status( | |
| state: tauri::State<AppState>, | |
| kind: String, | |
| connection_id: String, | |
| ) -> Option<ProviderConnectionStatus> { | |
| let store = state.provider_status.lock().unwrap(); | |
| provider_status_for_kind(&store, kind.trim()) | |
| .and_then(|statuses| statuses.get(connection_id.trim()).cloned()) | |
| } |
| #[tauri::command] | ||
| async fn test_asr_connection( | ||
| app: tauri::AppHandle, | ||
| request: ConnectionTestRequest, | ||
| ) -> ConnectionTestResult { | ||
| let status_path = provider_status_path(&app); | ||
| let connection_id = request | ||
| .connection_id | ||
| .clone() | ||
| .map(|id| id.trim().to_string()) | ||
| .filter(|id| !id.is_empty()) | ||
| .unwrap_or_else(|| "default".into()); | ||
| let provider = request.provider.trim().to_string(); | ||
| let result = match provider.as_str() { | ||
| "mock" => Ok("Mock ASR is available.".to_string()), | ||
| "openai-compatible" | "" => test_openai_compatible_asr(request).await, | ||
| other => Err(anyhow::anyhow!("unsupported ASR provider: {}", other)), | ||
| }; | ||
| let mut result = connection_test_result(provider, result); | ||
| if result.ok && result.provider == "mock" { | ||
| result.message_key = Some("settings.mock_asr_ok".into()); | ||
| } | ||
| result.tested_at = Some(chrono::Utc::now().to_rfc3339()); | ||
| save_connection_test_status(&status_path, "asr", &connection_id, &result); | ||
| result | ||
| } |
There was a problem hiding this comment.
Use tauri::State<'_, AppState> to access the in-memory provider_status store and eliminate the race condition on concurrent connection tests.
#[tauri::command]
async fn test_asr_connection(
state: tauri::State<'_, AppState>,
request: ConnectionTestRequest,
) -> ConnectionTestResult {
let connection_id = request
.connection_id
.clone()
.map(|id| id.trim().to_string())
.filter(|id| !id.is_empty())
.unwrap_or_else(|| "default".into());
let provider = request.provider.trim().to_string();
let result = match provider.as_str() {
"mock" => Ok("Mock ASR is available.".to_string()),
"openai-compatible" | "" => test_openai_compatible_asr(request).await,
other => Err(anyhow::anyhow!("unsupported ASR provider: {}", other)),
};
let mut result = connection_test_result(provider, result);
if result.ok && result.provider == "mock" {
result.message_key = Some("settings.mock_asr_ok".into());
}
result.tested_at = Some(chrono::Utc::now().to_rfc3339());
save_connection_test_status(&state, "asr", &connection_id, &result);
result
}| #[tauri::command] | ||
| async fn test_llm_connection( | ||
| app: tauri::AppHandle, | ||
| request: ConnectionTestRequest, | ||
| ) -> ConnectionTestResult { | ||
| let status_path = provider_status_path(&app); | ||
| let connection_id = request | ||
| .connection_id | ||
| .clone() | ||
| .map(|id| id.trim().to_string()) | ||
| .filter(|id| !id.is_empty()) | ||
| .unwrap_or_else(|| "default".into()); | ||
| let provider = request.provider.trim().to_string(); | ||
| let result = match provider.as_str() { | ||
| "mock" | "" => Ok("Mock LLM is available.".to_string()), | ||
| "openai-compatible" => test_openai_compatible_llm(request).await, | ||
| other => Err(anyhow::anyhow!("unsupported LLM provider: {}", other)), | ||
| }; | ||
| let mut result = connection_test_result(provider, result); | ||
| if result.ok && result.provider == "mock" { | ||
| result.message_key = Some("settings.mock_llm_ok".into()); | ||
| } | ||
| result.tested_at = Some(chrono::Utc::now().to_rfc3339()); | ||
| save_connection_test_status(&status_path, "llm", &connection_id, &result); | ||
| result | ||
| } |
There was a problem hiding this comment.
Use tauri::State<'_, AppState> to access the in-memory provider_status store and eliminate the race condition on concurrent connection tests.
#[tauri::command]
async fn test_llm_connection(
state: tauri::State<'_, AppState>,
request: ConnectionTestRequest,
) -> ConnectionTestResult {
let connection_id = request
.connection_id
.clone()
.map(|id| id.trim().to_string())
.filter(|id| !id.is_empty())
.unwrap_or_else(|| "default".into());
let provider = request.provider.trim().to_string();
let result = match provider.as_str() {
"mock" | "" => Ok("Mock LLM is available.".to_string()),
"openai-compatible" => test_openai_compatible_llm(request).await,
other => Err(anyhow::anyhow!("unsupported LLM provider: {}", other)),
};
let mut result = connection_test_result(provider, result);
if result.ok && result.provider == "mock" {
result.message_key = Some("settings.mock_llm_ok".into());
}
result.tested_at = Some(chrono::Utc::now().to_rfc3339());
save_connection_test_status(&state, "llm", &connection_id, &result);
result
}| fn save_connection_test_status( | ||
| path: &Path, | ||
| kind: &str, | ||
| connection_id: &str, | ||
| result: &ConnectionTestResult, | ||
| ) { | ||
| let mut store = load_provider_status(path); | ||
| if let Some(statuses) = provider_status_for_kind_mut(&mut store, kind) { | ||
| statuses.insert(connection_id.to_string(), test_result_to_status(result)); | ||
| if let Err(e) = save_provider_status(path, &store) { | ||
| tracing::warn!( | ||
| "failed to save provider status to {}: {}", | ||
| path.display(), | ||
| e | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Update save_connection_test_status to lock and write to the in-memory provider_status store in AppState instead of performing unsafe concurrent file reads and writes.
| fn save_connection_test_status( | |
| path: &Path, | |
| kind: &str, | |
| connection_id: &str, | |
| result: &ConnectionTestResult, | |
| ) { | |
| let mut store = load_provider_status(path); | |
| if let Some(statuses) = provider_status_for_kind_mut(&mut store, kind) { | |
| statuses.insert(connection_id.to_string(), test_result_to_status(result)); | |
| if let Err(e) = save_provider_status(path, &store) { | |
| tracing::warn!( | |
| "failed to save provider status to {}: {}", | |
| path.display(), | |
| e | |
| ); | |
| } | |
| } | |
| } | |
| fn save_connection_test_status( | |
| state: &AppState, | |
| kind: &str, | |
| connection_id: &str, | |
| result: &ConnectionTestResult, | |
| ) { | |
| let mut store = state.provider_status.lock().unwrap(); | |
| if let Some(statuses) = provider_status_for_kind_mut(&mut store, kind) { | |
| statuses.insert(connection_id.to_string(), test_result_to_status(result)); | |
| if let Err(e) = save_provider_status(&state.provider_status_path, &store) { | |
| tracing::warn!( | |
| "failed to save provider status to {}: {}", | |
| state.provider_status_path.display(), | |
| e | |
| ); | |
| } | |
| } | |
| } |
| .setup(move |app| { | ||
| // Load or create config | ||
| let cfg_path = config_path(app.handle()); | ||
| let status_path = provider_status_path(app.handle()); |
| app.manage(AppState { | ||
| config: Mutex::new(config), | ||
| config_path: cfg_path.clone(), | ||
| provider_status_path: status_path, |
| async function testAsrConnection() { | ||
| if (!validateSettings()) return; | ||
| saveAsrConnectionFields(); | ||
| setConnectionTestStatus('asr', 'testing', typexI18n.t('settings.connection_testing'), new Date().toISOString()); | ||
| try { | ||
| const result = await invoke('test_asr_connection', { request: getAsrConnectionDraft() }); | ||
| renderConnectionTestStatus('asr', result); | ||
| } catch (e) { | ||
| renderConnectionTestStatus('asr', { | ||
| ok: false, | ||
| provider: document.getElementById('asr-provider').value || 'mock', | ||
| message: String(e || typexI18n.t('settings.connection_failed')), | ||
| tested_at: new Date().toISOString(), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Await saveConfig() before running the connection test. This ensures that the draft settings are persisted to disk and any stale status is correctly reset before the test runs, preventing UI state mismatches.
| async function testAsrConnection() { | |
| if (!validateSettings()) return; | |
| saveAsrConnectionFields(); | |
| setConnectionTestStatus('asr', 'testing', typexI18n.t('settings.connection_testing'), new Date().toISOString()); | |
| try { | |
| const result = await invoke('test_asr_connection', { request: getAsrConnectionDraft() }); | |
| renderConnectionTestStatus('asr', result); | |
| } catch (e) { | |
| renderConnectionTestStatus('asr', { | |
| ok: false, | |
| provider: document.getElementById('asr-provider').value || 'mock', | |
| message: String(e || typexI18n.t('settings.connection_failed')), | |
| tested_at: new Date().toISOString(), | |
| }); | |
| } | |
| } | |
| async function testAsrConnection() { | |
| if (!validateSettings()) return; | |
| if (!await saveConfig()) return; | |
| setConnectionTestStatus('asr', 'testing', typexI18n.t('settings.connection_testing'), new Date().toISOString()); | |
| try { | |
| const result = await invoke('test_asr_connection', { request: getAsrConnectionDraft() }); | |
| renderConnectionTestStatus('asr', result); | |
| } catch (e) { | |
| renderConnectionTestStatus('asr', { | |
| ok: false, | |
| provider: document.getElementById('asr-provider').value || 'mock', | |
| message: String(e || typexI18n.t('settings.connection_failed')), | |
| tested_at: new Date().toISOString(), | |
| }); | |
| } | |
| } |
| async function testLlmConnection() { | ||
| if (!validateSettings()) return; | ||
| saveLlmConnectionFields(); | ||
| setConnectionTestStatus('llm', 'testing', typexI18n.t('settings.connection_testing'), new Date().toISOString()); | ||
| try { | ||
| const result = await invoke('test_llm_connection', { request: getLlmConnectionDraft() }); | ||
| renderConnectionTestStatus('llm', result); | ||
| } catch (e) { | ||
| renderConnectionTestStatus('llm', { | ||
| ok: false, | ||
| provider: document.getElementById('llm-provider').value || 'mock', | ||
| message: String(e || typexI18n.t('settings.connection_failed')), | ||
| tested_at: new Date().toISOString(), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Await saveConfig() before running the connection test. This ensures that the draft settings are persisted to disk and any stale status is correctly reset before the test runs, preventing UI state mismatches.
| async function testLlmConnection() { | |
| if (!validateSettings()) return; | |
| saveLlmConnectionFields(); | |
| setConnectionTestStatus('llm', 'testing', typexI18n.t('settings.connection_testing'), new Date().toISOString()); | |
| try { | |
| const result = await invoke('test_llm_connection', { request: getLlmConnectionDraft() }); | |
| renderConnectionTestStatus('llm', result); | |
| } catch (e) { | |
| renderConnectionTestStatus('llm', { | |
| ok: false, | |
| provider: document.getElementById('llm-provider').value || 'mock', | |
| message: String(e || typexI18n.t('settings.connection_failed')), | |
| tested_at: new Date().toISOString(), | |
| }); | |
| } | |
| } | |
| async function testLlmConnection() { | |
| if (!validateSettings()) return; | |
| if (!await saveConfig()) return; | |
| setConnectionTestStatus('llm', 'testing', typexI18n.t('settings.connection_testing'), new Date().toISOString()); | |
| try { | |
| const result = await invoke('test_llm_connection', { request: getLlmConnectionDraft() }); | |
| renderConnectionTestStatus('llm', result); | |
| } catch (e) { | |
| renderConnectionTestStatus('llm', { | |
| ok: false, | |
| provider: document.getElementById('llm-provider').value || 'mock', | |
| message: String(e || typexI18n.t('settings.connection_failed')), | |
| tested_at: new Date().toISOString(), | |
| }); | |
| } | |
| } |
Keep provider connection status in memory while persisting it to disk, and preserve the testing UI state while saving pending settings before a connection test. Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces connection testing functionality for ASR and LLM providers, adding UI elements, localized strings, and backend Tauri commands to test connections and persist their status. The review feedback highlights three key areas for improvement: checking for NaN when parsing timestamps in the frontend to prevent incorrect relative time formatting, refactoring unstable let-chains in Rust to ensure compatibility with stable compilers, and offloading synchronous file operations to tokio::task::spawn_blocking to avoid blocking the async executor threads.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function formatRelativeTestTime(timestamp) { | ||
| if (!timestamp) return typexI18n.t('settings.connection_not_tested'); | ||
| const time = typeof timestamp === 'number' ? timestamp : Date.parse(timestamp); | ||
| const elapsedMs = Date.now() - time; | ||
| if (!Number.isFinite(elapsedMs) || elapsedMs < 60_000) { | ||
| return typexI18n.t('settings.connection_last_tested_just_now'); | ||
| } |
There was a problem hiding this comment.
If timestamp is an invalid date string, Date.parse(timestamp) returns NaN. Since Number.isFinite(NaN) is false, the condition !Number.isFinite(elapsedMs) evaluates to true, causing the function to incorrectly return "Last tested just now" instead of handling the parsing failure. Adding a check for Number.isNaN(time) prevents this bug.
| function formatRelativeTestTime(timestamp) { | |
| if (!timestamp) return typexI18n.t('settings.connection_not_tested'); | |
| const time = typeof timestamp === 'number' ? timestamp : Date.parse(timestamp); | |
| const elapsedMs = Date.now() - time; | |
| if (!Number.isFinite(elapsedMs) || elapsedMs < 60_000) { | |
| return typexI18n.t('settings.connection_last_tested_just_now'); | |
| } | |
| function formatRelativeTestTime(timestamp) { | |
| if (!timestamp) return typexI18n.t('settings.connection_not_tested'); | |
| const time = typeof timestamp === 'number' ? timestamp : Date.parse(timestamp); | |
| if (Number.isNaN(time)) return typexI18n.t('settings.connection_not_tested'); | |
| const elapsedMs = Date.now() - time; | |
| if (elapsedMs < 60_000) { | |
| return typexI18n.t('settings.connection_last_tested_just_now'); | |
| } |
| let mut provider_status = state.provider_status.lock().unwrap(); | ||
| if reset_changed_provider_status(&old_config, &config, &mut provider_status) | ||
| && let Err(e) = save_provider_status(&state.provider_status_path, &provider_status) | ||
| { | ||
| tracing::warn!( | ||
| "failed to save provider status to {}: {}", | ||
| state.provider_status_path.display(), | ||
| e | ||
| ); | ||
| } |
There was a problem hiding this comment.
The use of && let (let-chains) is an unstable Rust feature (RFC 2497) and requires #![feature(let_chains)] to compile. To ensure compatibility with stable Rust and improve readability, this should be refactored into nested if statements.
| let mut provider_status = state.provider_status.lock().unwrap(); | |
| if reset_changed_provider_status(&old_config, &config, &mut provider_status) | |
| && let Err(e) = save_provider_status(&state.provider_status_path, &provider_status) | |
| { | |
| tracing::warn!( | |
| "failed to save provider status to {}: {}", | |
| state.provider_status_path.display(), | |
| e | |
| ); | |
| } | |
| let mut provider_status = state.provider_status.lock().unwrap(); | |
| if reset_changed_provider_status(&old_config, &config, &mut provider_status) { | |
| if let Err(e) = save_provider_status(&state.provider_status_path, &provider_status) { | |
| tracing::warn!( | |
| "failed to save provider status to {}: {}", | |
| state.provider_status_path.display(), | |
| e | |
| ); | |
| } | |
| } |
| let mut store = state.provider_status.lock().unwrap(); | ||
| if let Some(statuses) = provider_status_for_kind_mut(&mut store, kind) { | ||
| statuses.insert(connection_id.to_string(), test_result_to_status(result)); | ||
| if let Err(e) = save_provider_status(&state.provider_status_path, &store) { | ||
| tracing::warn!( | ||
| "failed to save provider status to {}: {}", | ||
| state.provider_status_path.display(), | ||
| e | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Performing synchronous file I/O (save_provider_status) directly inside an async context (called by the async commands test_asr_connection and test_llm_connection) can block the Tokio executor threads. Offloading this blocking operation to tokio::task::spawn_blocking is highly recommended to maintain responsiveness.
| let mut store = state.provider_status.lock().unwrap(); | |
| if let Some(statuses) = provider_status_for_kind_mut(&mut store, kind) { | |
| statuses.insert(connection_id.to_string(), test_result_to_status(result)); | |
| if let Err(e) = save_provider_status(&state.provider_status_path, &store) { | |
| tracing::warn!( | |
| "failed to save provider status to {}: {}", | |
| state.provider_status_path.display(), | |
| e | |
| ); | |
| } | |
| } | |
| let mut store = state.provider_status.lock().unwrap(); | |
| if let Some(statuses) = provider_status_for_kind_mut(&mut store, kind) { | |
| statuses.insert(connection_id.to_string(), test_result_to_status(result)); | |
| let store_clone = store.clone(); | |
| let path_clone = state.provider_status_path.clone(); | |
| tokio::task::spawn_blocking(move || { | |
| if let Err(e) = save_provider_status(&path_clone, &store_clone) { | |
| tracing::warn!( | |
| "failed to save provider status to {}: {}", | |
| path_clone.display(), | |
| e | |
| ); | |
| } | |
| }); | |
| } |
Treat unparsable provider status timestamps as not tested instead of showing them as just tested. Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces connection testing functionality for both ASR and LLM providers in the desktop application. It adds UI elements, CSS styling, and localization keys for testing connections, alongside Tauri backend commands to perform the actual connection tests (including mock and OpenAI-compatible endpoints) and persist their status. Feedback on the backend implementation highlights two critical issues in apps/desktop/src/lib.rs: first, using String::truncate with a hardcoded byte limit on error messages can cause panics on multi-byte UTF-8 characters; second, the use of the unstable let_chains feature (&& let) will cause compilation failures on stable Rust and should be refactored into nested if let statements.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if message.len() > 500 { | ||
| message.truncate(500); | ||
| message.push('…'); | ||
| } |
There was a problem hiding this comment.
Using String::truncate with a hardcoded byte limit can cause a panic if the truncation index falls in the middle of a multi-byte UTF-8 character (e.g., Chinese characters or emojis in error messages). It is safer to find the nearest valid character boundary before truncating.
if message.len() > 500 {
let mut limit = 500;
while !message.is_char_boundary(limit) {
limit -= 1;
}
message.truncate(limit);
message.push('…');
}| if reset_changed_provider_status(&old_config, &config, &mut provider_status) | ||
| && let Err(e) = save_provider_status(&state.provider_status_path, &provider_status) | ||
| { | ||
| tracing::warn!( | ||
| "failed to save provider status to {}: {}", | ||
| state.provider_status_path.display(), | ||
| e | ||
| ); | ||
| } |
There was a problem hiding this comment.
The let_chains feature (&& let Err(e) = ...) is currently unstable in Rust. To ensure compatibility with stable Rust compilers, it is recommended to use nested if let statements instead.
| if reset_changed_provider_status(&old_config, &config, &mut provider_status) | |
| && let Err(e) = save_provider_status(&state.provider_status_path, &provider_status) | |
| { | |
| tracing::warn!( | |
| "failed to save provider status to {}: {}", | |
| state.provider_status_path.display(), | |
| e | |
| ); | |
| } | |
| if reset_changed_provider_status(&old_config, &config, &mut provider_status) { | |
| if let Err(e) = save_provider_status(&state.provider_status_path, &provider_status) { | |
| tracing::warn!( | |
| "failed to save provider status to {}: {}", | |
| state.provider_status_path.display(), | |
| e | |
| ); | |
| } | |
| } |
Avoid truncating provider connection error messages in the middle of a UTF-8 character boundary. Co-Authored-By: Claude <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces connection testing capabilities for ASR and LLM providers, allowing users to verify their configuration settings directly from the desktop application's settings view. It adds the necessary UI elements, localization keys, and backend Tauri commands to trigger and track connection tests. The feedback focuses on improving application responsiveness and robustness by offloading synchronous file I/O to a blocking thread and adding timeouts to the network requests for OpenAI-compatible providers to prevent the UI from hanging indefinitely.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if let Err(e) = save_provider_status(&state.provider_status_path, &store) { | ||
| tracing::warn!( | ||
| "failed to save provider status to {}: {}", | ||
| state.provider_status_path.display(), | ||
| e | ||
| ); | ||
| } |
There was a problem hiding this comment.
The save_provider_status function performs synchronous file I/O (std::fs::write). Calling this directly on the async executor thread in save_connection_test_status blocks the thread, which can degrade application responsiveness. Since ProviderStatusStore implements Clone, we can clone the store, drop the lock guard early, and perform the file write inside tokio::task::spawn_blocking to keep the async executor thread free.
let path = state.provider_status_path.clone();
let store_clone = store.clone();
drop(store);
tokio::task::spawn_blocking(move || {
if let Err(e) = save_provider_status(&path, &store_clone) {
tracing::warn!(
"failed to save provider status to {}: {}",
path.display(),
e
);
}
});| let result = match provider.as_str() { | ||
| "mock" => Ok("Mock ASR is available.".to_string()), | ||
| "openai-compatible" | "" => test_openai_compatible_asr(request).await, | ||
| other => Err(anyhow::anyhow!("unsupported ASR provider: {}", other)), | ||
| }; |
There was a problem hiding this comment.
The connection test for the OpenAI-compatible ASR provider does not have a timeout. If the configured endpoint is unresponsive or hangs, the test will block indefinitely, keeping the UI in a 'testing' state. Wrapping the async call in tokio::time::timeout ensures the test fails gracefully after a reasonable duration (e.g., 30 seconds).
| let result = match provider.as_str() { | |
| "mock" => Ok("Mock ASR is available.".to_string()), | |
| "openai-compatible" | "" => test_openai_compatible_asr(request).await, | |
| other => Err(anyhow::anyhow!("unsupported ASR provider: {}", other)), | |
| }; | |
| let result = match provider.as_str() { | |
| "mock" => Ok("Mock ASR is available.".to_string()), | |
| "openai-compatible" | "" => { | |
| match tokio::time::timeout(std::time::Duration::from_secs(30), test_openai_compatible_asr(request)).await { | |
| Ok(res) => res, | |
| Err(_) => Err(anyhow::anyhow!("Connection timed out after 30 seconds")), | |
| } | |
| } | |
| other => Err(anyhow::anyhow!("unsupported ASR provider: {}", other)), | |
| }; |
| let result = match provider.as_str() { | ||
| "mock" | "" => Ok("Mock LLM is available.".to_string()), | ||
| "openai-compatible" => test_openai_compatible_llm(request).await, | ||
| other => Err(anyhow::anyhow!("unsupported LLM provider: {}", other)), | ||
| }; |
There was a problem hiding this comment.
The connection test for the OpenAI-compatible LLM provider does not have a timeout. If the configured endpoint is unresponsive or hangs, the test will block indefinitely, keeping the UI in a 'testing' state. Wrapping the async call in tokio::time::timeout ensures the test fails gracefully after a reasonable duration (e.g., 30 seconds).
| let result = match provider.as_str() { | |
| "mock" | "" => Ok("Mock LLM is available.".to_string()), | |
| "openai-compatible" => test_openai_compatible_llm(request).await, | |
| other => Err(anyhow::anyhow!("unsupported LLM provider: {}", other)), | |
| }; | |
| let result = match provider.as_str() { | |
| "mock" | "" => Ok("Mock LLM is available.".to_string()), | |
| "openai-compatible" => { | |
| match tokio::time::timeout(std::time::Duration::from_secs(30), test_openai_compatible_llm(request)).await { | |
| Ok(res) => res, | |
| Err(_) => Err(anyhow::anyhow!("Connection timed out after 30 seconds")), | |
| } | |
| } | |
| other => Err(anyhow::anyhow!("unsupported LLM provider: {}", other)), | |
| }; |
Summary
provider_status.jsonfile.🤖 Generated with Claude Code